-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Resolve Deppy error check todo #535
🐛 Resolve Deppy error check todo #535
Conversation
262f36f
to
088c9b7
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
==========================================
- Coverage 84.72% 84.53% -0.19%
==========================================
Files 23 23
Lines 910 912 +2
==========================================
Hits 771 771
- Misses 95 96 +1
- Partials 44 45 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
func prettyUnsatMessage(err error) string { | ||
unsat := deppy.NotSatisfiable{} | ||
if !errors.As(err, &unsat) { | ||
panic("we must always receive deppy.NotSatisfiable here") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always hesitant about introducing panics in code, could you explain the benefit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prettyUnsatMessage
is a workaround operator-framework/deppy#142 (there is a comment about it above, not in the diff). It will go away and we will be doing something like this in controller code eventually:
setResolvedStatusConditionFailed(&op.Status.Conditions, err.Error(), op.GetGeneration())
I was thinking failing laudly given that this is a temporary code and this condition should never happen unless we introduce a regression. But now I'm thinking about it again - yes, probably there is not much benefit in this and we can do just something like this:
unsat := deppy.NotSatisfiable{}
if !errors.As(err, &unsat) {
return err.Error()
}
I'll update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
We already have a version which contains the fix and can revert to a simpler check. Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
088c9b7
to
7fb8b6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
3373785
Description
We already have a version which contains the fix and can revert to a simpler check.
Reviewer Checklist